Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Feature/corporate maven repository #836

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Feature/corporate maven repository #836

wants to merge 6 commits into from
+492 −50

Conversation

gonzalad
Copy link

@gonzalad gonzalad commented Oct 16, 2024 •
edited
Loading

Hello, this PR should make it easier to use the library with a corporate maven repository.

The PR addresses:

@wing328 or @kay-schecker is it possible for you to review this PR please ?

Contents of this PR:

  1. fix NO_PROXY issue
    This is done by removing explicit handling of HTTP_PROXY and HTTPS_PROXY: Axios already handles that (see https://github.com/axios/axios#request-config).
  2. added generator-cli.http.rejectUnauthorized configuration to avoid the error unable to get local issuer certificate.
  3. support using placeholders for environment variables in any configuration value.
    this will allow to use env vars to set credentials in the queryUrl or downloadUrl:
    Usage: 
    {
  "$schema": "./node_modules/@openapitools/openapi-generator-cli/config.schema.json",
  "spaces": 2,
  "generator-cli": {
    "version": "7.9.0",
    "repository": {
      "downloadUrl": "https://${MVN_USERNAME}:${MVN_PASSWORD}@mynexus.com/repository/maven-proxy-group/${groupId}/${artifactId}/${versionName}/${artifactId}-${versionName}.jar"
    }
  }
}
  4. For people using a nexus registry for both npm and maven registry: add support for retrieving authToken in .npmrc and using the npm token to authenticate to the maven repository.
    • the credentials for connecting to maven repository (authToken) are retrieved from .npmrc file
      (the credentials used is the one matching the url of the maven server).
    • also, if strict-ssl is set to false in npmrc, then certificate verification is disabled
      Usage:
    {
  "$schema": "./node_modules/@openapitools/openapi-generator-cli/config.schema.json",
  "spaces": 2,
  "generator-cli": {
    "version": "7.9.0",
    "repository": {
      "downloadUrl": "https://mynexus.com/repository/maven-proxy-group/${groupId}/${artifactId}/${versionName}/${artifactId}-${versionName}.jar"
    },
    "useNpmrc": true
  }
}
  5. Fix for running unit tests on windows
  6. Activates logging (I had some issues testing the library on a project, and the library didn't output any logs)

GONZALEZ Adrian added 6 commits October 13, 2024 22:58
feat(http): restore axios default proxy handling
67d713f 
The current implementation does not handle NO_PROXY env var.

I removed explicit proxy handling, since axios already
handle proxy related environment variables.

See https://github.com/axios/axios#request-config:

`proxy` defines the hostname, port, and protocol of the proxy server.
You can also define your proxy using the conventional `http_proxy` and
`https_proxy` environment variables. If you are using environment
variables
for your proxy configuration, you can also define a `no_proxy`
environment
variable as a comma-separated list of domains that should not be
proxied.
feat: activate logging
9dc53a6
feat(http): add rejectUnauthorized configuration
58df572 
This helps to avoid the error `unable to get local issuer certificate`
when using a corporate maven repository.

Another solution would be to declare the env var
NODE_TLS_REJECT_UNAUTHORIZED=0.
test: fix tests on windows
a7e54a7
feat(config): add support for environment variable placeholders
5cf18ed 
- Implemented replaceEnvPlaceholders function to replace ${VAR}
  placeholders in config values with corresponding environment variable
  values.
- If the environment variable does not exist, the placeholder remains
  unchanged.
feat: add support for querying Maven repository using npmrc settings
9a09e94 
- Implemented functionality to query Maven repository using authToken
  and strict-ssl settings from npmrc.
- Updated httpModuleConfigFactory to configure HTTPS agent based on
  npmrc settings.
- Enhanced NpmrcService to retrieve authToken and strict-ssl
  settings.
@wing328
Copy link
Member

wing328 commented Oct 17, 2024

Thanks for the PR but your commit (as shown in the Commits tab) is not linked to your Github account, which means this PR won't count as your contribution in https://github.com/OpenAPITools/openapi-generator/graphs/contributors.

Let me know if you need help fixing it.

Ref: https://github.com/OpenAPITools/openapi-generator/wiki/FAQ#how-can-i-update-commits-that-are-not-linked-to-my-github-account

@wing328
Copy link
Member

wing328 commented Oct 17, 2024 •
edited
Loading

thanks for the PR

I wonder if you can break down this PR into 2 smaller PRs to address the following issues separately for easier review:

@gonzalad
Copy link
Author

thanks for the PR

I wonder if you can break down this PR into 2 smaller PRs to address the following issues separately for easier review:

Sure
I'll break it down in 2 PRs this evening and will close this PR one once it is done.
I'll fix the commit link to my github account at the same time

Thanks !

@gonzalad
Copy link
Author

gonzalad commented Oct 17, 2024 •
edited
Loading

Default proxy handling in axios has an issue (returns 400 Bad Request with some proxies), see axios/axios#5256.

Sorry, I didn't see that issue before.

Now, I understand why you didn't us the default proxy handling from axios.

This PR should fix the issue #781.

I did some basic tests with some similar code, and it works.

I'll do more tests with the code from this PR and will keep you informed.

@gonzalad
Copy link
Author

thanks for the PR

I wonder if you can break down this PR into 2 smaller PRs to address the following issues separately for easier review:

@wing328 : I tested PR #781 in my corporate environment and it works great (it supports NO_PROXY handling), could #781 be merged ?

@wing328
Copy link
Member

wing328 commented Oct 18, 2024

@wing328 : I tested PR #781 in my corporate environment and it works great (it supports NO_PROXY handling), could #781 be merged ?

thanks for testing it

will merge once the merge conflicts are resolved

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@gonzalad @wing328